Fix issue and persist a new refresh token when revoking refresh token is disabled #1449
Conversation
|
I don't think this was actually a bug. I had intended not to issue a new refresh token as the old one was still valid and in this case, the issuing of a new refresh token is entirely optional. However, I think it is considered good practice to always issue a new refresh token as you can then detect bad actors by noticing when the same refresh token is used twice so happy for this to be merged in. Like the other PR, would it be possible to add some tests to support this and a changelog entry? If you don't have time please let me know and I can take a stab at this. Thank you |
@Sephster The old refresh token has the old access token ID encrypted in it, which is valid for the old access token but not for the new access token, so we always need to issue / persist a new refresh token for the new access token. PS 1: There is already another way to disable issuing refresh token by returning |
|
Getting away from the topic at hand a wee bit but I can't actually see why we encrypt the access token in the refresh token. I might be missing something but it doesn't look like it is used anywhere in core code at least. Apologies if I'm missing something obvious (it is late at night) but again, I don't think it matters if we don't issue a new refresh token from a functional point. We should be able to use the old one indefinitely even if it contains an access token that has long since been revoked although I understand how this could cause some confusion. I still think this is a worthwhile change, just mulling over the changelog entry and thinking out loud more than anything. I will hopefully merge this in shortly. Thanks @hafezdivandari |
|
ahh no I see now. We use it to revoke the old access token when the refresh token is used. Didn't spot that initially. All good. Thanks @hafezdivandari |
A bug was introduced in version 8.3.0 via #1189, which became apparent after the addition of the
GrantTypeInterface::revokeRefreshTokens()function in version 9.0.When disabling refresh token revocation with
revokeRefreshTokens(false), the intention is to avoid revoking the old refresh token during an access token refresh, allowing reuse if the response isn't received (e.g., due to a network issue).While the old refresh token is not revoked, this setting also prevents issuing a new refresh token for the refreshed access token, which is not the intended behavior.
Please refer to the related PR and the failed tests on Laravel Passport for more details: laravel/passport#1790